Skip to content

feat(server): use shared model-agnostic canonical request/response types#307

Open
sercand wants to merge 9 commits into
Avarok-Cybersecurity:mainfrom
sercand:feat/canonical-chat-ir
Open

feat(server): use shared model-agnostic canonical request/response types#307
sercand wants to merge 9 commits into
Avarok-Cybersecurity:mainfrom
sercand:feat/canonical-chat-ir

Conversation

@sercand

@sercand sercand commented Jul 13, 2026

Copy link
Copy Markdown

This PR is quite big, sorry for putting everything into single PR.

Summary

Introduces a canonical, model-agnostic chat IR (crates/spark-server/src/ir/) and migrates every API surface to it, so the core pipeline (build_msg_entries, template rendering, scheduler) consumes one provider-free representation instead of the OpenAI wire structs. Each protocol becomes a thin, unit-tested adapter pair: openai/to_ir.rs / anthropic/to_ir.rs on the request side, openai/encode.rs + encode_stream.rs, anthropic/translate.rs (ir_to_anthropic_response) + the translator.rs streaming state machine, and typed responses_translate.rs / responses_stream.rs on the response side.

Because Message.content is now always a list of parts for every role, the motivating bug — images attached to tool-result messages being silently dropped — falls out for free, along with the rest of the OpenAI-shape tax: the Anthropic struct→JSON→struct round-trip is deleted, the Responses API no longer drops images, and the <think>\n\n</think> and [tool error]\n string-smuggling hacks become real fields (reasoning, an error flag on tool results). The migration commit also fixes a scheduler bug surfaced by end-to-end verification of the tool+image agent loop: the post-think EOS guard fired on every tools-armed turn (eating the model's natural <|im_end|> and causing scaffold garbage plus spurious duplicate tool calls); it is now gated on a sticky think_force_closed flag so it only applies after a watchdog/budget force-injected </think>.

Closes #165 and #164

Test plan

  • cargo fmt --all -- --check
  • ATLAS_SKIP_BUILD=1 cargo clippy --workspace --tests -- -Dwarnings
  • bash scripts/check-license-headers.sh
  • Tested against a real model: live on Qwen3.6-35B-A3B-NVFP4 (grammar ON) — /v1/messages turn-2 tool replays 4/4 clean end_turn; full 2-turn tool+image agent loop 3/3 clean (previously 4 turns with chat-template scaffold garbage and duplicate tool calls); new scripts/test-qwen36-tool-image.sh drives the loop end-to-end
  • Added or updated tests: cargo test -p spark-server → 689 passed, 0 failed (30 lib + 659 bin). New suites: ir/tests.rs, anthropic/tests/ir_carry.rs (wire→IR→wire carry), anthropic/tests/translator_stream.rs (streaming-framing goldens), expanded openai/tests.rs; prompt-byte stability gates (prompt_json_stability_gate, rendered_prompt_json_matches_retired_json_hop_output) pin rendered prompts to the pre-migration output

Notes for reviewers

  • Invariants the tests pin: prompt bytes are the KV-cache prefix and wire framing is what agent clients execute, so two golden gates guard the migration — the rendered-prompt JSON gates (template + Anthropic retired-JSON-hop) and the Anthropic streaming goldens. Any future change to message lowering or stream framing should keep these green.
  • Echo-only wire fields (service_tier, metadata, store, include_usage) deliberately do not enter the IR; they ride api/chat/echo.rs::ResponseEcho handler-side.
  • Responses API lowers to the chat wire inside openai/ before hitting the IR — intra-provider normalization, deliberate; it now carries images instead of dropping them (from_responses_input_item).
  • Accepted wire references outside the adapter edges: response_store (caches wire bodies) and chat_stream --dump synthesis.
  • Deferred follow-ups: a neutral ApiError type; top-level reasoning_effort remains dead wire-compat (never wired into the thinking ladder).
  • count_tokens now shares api/chat/prepare.rs::prepare_chat_prompt with the serving path (measured end-to-end drift 0.0%).

CLA

Introduce crates/spark-server/src/ir/ — a provider-free chat IR
(ChatRequest, Message with content always a list of parts,
ChatResponse, StreamDelta) — and migrate every API surface onto it:
openai/to_ir.rs + anthropic/to_ir.rs adapt requests in; encode.rs /
encode_stream.rs / translate.rs / responses_translate.rs serialize
responses out. Deletes the Anthropic struct→JSON→struct round-trip,
the Responses-API image drop, and the <think> / [tool error] string
prefixes (now real fields). Images ride every role uniformly,
including tool results (issue Avarok-Cybersecurity#165).

Also scopes the post-think EOS guard to force-closed </think> spans
(sticky ActiveSeq::think_force_closed): it previously fired on every
tools-armed turn, eating the model's natural <|im_end|> after brief
post-tool answers and causing scaffold garbage + spurious duplicate
tool calls. Verified live on Qwen3.6-35B-A3B-NVFP4: turn-2 /v1/messages
replays 4/4 clean end_turn; 2-turn tool+image agent loop 3/3 clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

All contributors have signed the CLA. Thank you!
Posted by the CLA Assistant Lite bot.

@sercand

sercand commented Jul 13, 2026

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@tbraun96

Copy link
Copy Markdown
Contributor

@sercand this looks like a really great PR, thanks! I will need to run some benchmarks to ensure no regressions.

@tbraun96 tbraun96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ensure future code follows Rust conventions, it would be best to change things up a bit. There may be more locations in your code where the fixes would also apply.

Comment thread crates/spark-server/src/anthropic/handlers.rs Outdated
Comment thread crates/spark-server/src/anthropic/to_ir.rs Outdated
Comment thread crates/spark-server/src/anthropic/translate.rs Outdated
Comment thread crates/spark-server/src/anthropic/translator.rs Outdated
Comment thread crates/spark-server/src/anthropic/types.rs Outdated
Comment thread crates/spark-server/src/api/chat/echo.rs Outdated
@sercand

sercand commented Jul 15, 2026

Copy link
Copy Markdown
Author

@tbraun96 please have a look at again 🙏

sercand and others added 2 commits July 16, 2026 08:23
Trunk brought per-request LoRA routing (M2), NLLB encoder-decoder
support (src/tgt langs + beam search), per-request loop-detector
overrides, P0-3 corrective tool-call feedback, and P1-6 duplicate-error
masking — all landed at the old wire-level chat path. Resolutions port
them onto the canonical IR:

- ir::ChatRequest grows adapter/src_lang/tgt_lang/num_beams/
  length_penalty/early_stopping; OpenAI adapter maps them, Anthropic
  pins None. chat_completions_inner resolves adapter slot, lang token
  ids, and beam params as on main (errors → ChatOutcome::Http).
- RepetitionDetectionParams stays at api/inference_types.rs (our move);
  main's in-place edit was a doc rewrap only.
- P0-3 corrective hint re-emitted as StreamDelta::Content instead of a
  raw SSE ChatCompletionChunk (message text byte-identical).
- P1-6 duplicate_error_masks kept alongside our IR build_tests; the
  masking runs pre-hint on MsgEntry text as on main.
- /v1/models handlers deleted from completions.rs in favor of main's
  LoRA-aware api/models.rs (retargeted to crate::ids::unix_timestamp).
- loop_detect P1-5 outcome pairing ported from wire messages to IR
  Message/Role; new prefill_a/b ActiveSeq initializers gain our
  think_force_closed field (false, matching sibling constructors).

spark-server: 704 bin + 30 lib tests pass, incl. the
prompt-JSON stability gate, anthropic retired-JSON golden, and
translator_stream framing goldens. Workspace check + clippy clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tbraun96
tbraun96 dismissed their stale review July 16, 2026 14:26

LGTM. Running tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feat] Canonical internal chat IR: convert every API surface into one model-agnostic request/response type (unblocks images in tool results)

2 participants